Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to make virtualenv metadata findable when invoked globally #207

Closed
wants to merge 11 commits into from

Conversation

kwentine
Copy link

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes

This PR is an attempt to fix #92 by:

  1. Detecting if deptry is likely run outside of the project's virtualenv
  2. If so, try to find the project's site-packages by checking common locations, and make it findable by importlib.metadata

To test the changes on the deptry project itself:

# Simulate a global deptry install
VENV=~/.virtualenvs/globalinstall
python -m venv $VENV
$VENV/bin/python -m pip install deptry
cd /path/to/deptry
# Should work
$VENV/bin/deptry .

@kwentine kwentine marked this pull request as ready for review November 18, 2022 17:18
@kwentine kwentine changed the title When invoked globally try to guess project virtual env When invoked globally try to guess project virtual env Nov 18, 2022
@kwentine kwentine changed the title When invoked globally try to guess project virtual env When invoked globally try to make project virtualenv metadata findable Nov 18, 2022
@kwentine kwentine changed the title When invoked globally try to make project virtualenv metadata findable Try to make project virtualenv metadata findable when invoked globally Nov 18, 2022
@kwentine kwentine changed the title Try to make project virtualenv metadata findable when invoked globally Try to make virtualenv metadata findable when invoked globally Nov 18, 2022
@fpgmaas
Copy link
Owner

fpgmaas commented Nov 18, 2022

Nice work, this seems to be a very good solution approach. I ran some tests locally and it seems to find the metadata in the virtual environment in all cases!

I will need some time to look into your proposed changes and see if I can provide some feedback, I will start now but will probably have to continue later this weekend.

The first thing I notice is the following:

➜  example-project git:(main) ✗ . /Users/florian/git/example-project/.venv/bin/activate
(example-project-py3.10) ➜  example-project git:(main) ✗ deptry .
example-project
Assuming project virtualenv /Users/florian/git/example-project/.venv/lib/python3.10/site-packages
Consider explicitly activating before running deptry
Scanning 2 files...
Success! No dependency issues found.
(example-project-py3.10) ➜  example-project git:(main) ✗ 

Here, I installed deptry globally. This error does not seem appropriate; I already activated my environment. Maybe we can try and detect the difference between a non-activated virtual environment, and a globally installed deptry version?

deptry/cli.py Outdated Show resolved Hide resolved
deptry/cli.py Outdated Show resolved Hide resolved
deptry/utils.py Outdated Show resolved Hide resolved
deptry/utils.py Outdated Show resolved Hide resolved
deptry/utils.py Outdated Show resolved Hide resolved
deptry/utils.py Outdated Show resolved Hide resolved
deptry/utils.py Outdated Show resolved Hide resolved
@fpgmaas
Copy link
Owner

fpgmaas commented Nov 18, 2022

Need to sign off for now since my train just arrived at the station, but just wanted to let you know I am impressed with your work! I hope I did not discourage you with my feedback. I look forward to merging this after one or two iterations :)

@kwentine
Copy link
Author

Thanks for your very kind and spot-on feedback :) I made an iteration during the limited time I had this morning, factoring out the logic in a dedicated virtualenv_finder module that exposes a simplified entrypoint to cli. Haven't addressed the argument cardinality and suggestion to refactor into classes yet.

Regarding your main comment concerning the warning message you were totally right: the issue is not whether the project virtualenv is active or not, but rather if we are being run by its interpreter.

Have a nice day :)

deptry/cli.py Outdated Show resolved Hide resolved
@kwentine
Copy link
Author

kwentine commented Nov 20, 2022

I'm not very knowledgeable with pre-commit, but as I understand it (and I think you mention it in the documentation), deptry installed as a hook may be the most common occurence of running outside of a project's virtualenv. For this use case the changes of this PR would be beneficial, since package metadata is likely to be found in standard layouts. But I remember that pre-commit allows to specify arguments to pass to hooks, so maybe supporting a --project-virtualenv argument in the CLI could be useful to users, and when provided, allow us to avoid the guesswork.

-   repo: https://github.com/fpgmaas/deptry.git
    rev: <tag>
    hooks:
    - id: deptry
      args:
        - "--skip-missing"
        - "--project-virtualenv path/to/virtualenv"

But that might be something for another PR :)

Copy link
Collaborator

@mkniewallner mkniewallner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think it's a nice option to be able to run deptry from a global installation, I must say I'm not a fan of the part that dynamically tries to guess where the virtual environment is, as the location is not standard, so the virtual environment can be created in various locations depending on the tool used.

For instance:

  • Poetry will by default create them in ~/.cache/pypoetry/virtualenvs on Linux
  • pyenv-virtualenv will by default create them in ~/.pyenv/versions on Linux

A more flexible choice might be to add a custom option to define the path to the virtual environment, like virtual-environment-path, since this would made the logic more simple and less magical, while allowing the flexibility that some users might need.

@mkniewallner
Copy link
Collaborator

mkniewallner commented Nov 21, 2022

A more flexible choice might be to add a custom option to define the path to the virtual environment, like virtual-environment-path, since this would made the logic more simple and less magical, while allowing the flexibility that some users might need.

mypy allows to do that through --python-executable option, where the user can pass the path to a Python executable (for instance mypy --python-executable ~/Library/Caches/pypoetry/virtualenvs/foobar-7GcI82l9-py3.10/bin/python) .

@kwentine
Copy link
Author

Thanks for your remarks @mkniewallner. I do agree that the guessing is brittle. One could argue that failing to find the virtualenv (because the candidate location list is incomplete) leaves the user no worse off than before. But maybe more worrisome is the risk to wrongly guess a virtualenv...

So @fpgmaas do you think should I directly work in this PR to add --python-site-packages CLI argument ? And maybe just emit a warning message if we suspect that deptry is being run globally without this argument provided ?

@fpgmaas
Copy link
Owner

fpgmaas commented Nov 21, 2022

@kwentine Yes, I think @mkniewallner point's are valid and his suggestion is a good one. Thanks mathieu for sharing your feedback! So then it would make sense to add the --python-site-packages CLI argument in this PR. I also agree with your suggestion to emit a message, I think that would be a clean solution.

@kwentine
Copy link
Author

Hi 👋🏻 Had shot at supporting --python-site-packages this morning. However I had some doubts: if users are accustomed to --python-executable from other tools as @mkniewallner remarked, maybe we should stick to that ? (And it is less cumbersome to specify since the python version is not in the path).

@kwentine
Copy link
Author

I also wanted to share an alternative, more explicit way of searching for metadata in a custom path, that occurred to me while perusing importlib.metadata source.

from importlib.metadata import Distribution
def distribution(name: str, path: str | None):
    kwargs = {'name': name}
    if path is not None:
        kwargs['path'] = [path] # or `[path, *sys.path]` ?
    try:
        return next(Distribution.discover(**kwargs))
    except StopIteration:
        raise PackageNotFoundError(name)

Using this function instead of metadata.distribution should do the trick, with the possible upsides of narrowing the search only to site packages (if that is wished) and being easier to test. At the cost of replacing all calls to importlib.metadata.distribution. Just an idea to toy with maybe :)

@fpgmaas
Copy link
Owner

fpgmaas commented Nov 23, 2022

Hi 👋🏻 Had shot at supporting --python-site-packages this morning. However I had some doubts: if users are accustomed to --python-executable from other tools as @mkniewallner remarked, maybe we should stick to that ? (And it is less cumbersome to specify since the python version is not in the path).

I think since we explicitly looking for site-packages, what you have currently implemented looks good to me. Otherwise, we still need to do some searching for the corresponding site-packages directory.

deptry/cli.py Outdated Show resolved Hide resolved
deptry/cli.py Outdated Show resolved Hide resolved
deptry/metadata_finder.py Outdated Show resolved Hide resolved


def install_metadata_finder(site_packages: Path) -> None:
"""Add poject virtualenv site packages to metadata search path"""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in project

@fpgmaas
Copy link
Owner

fpgmaas commented Mar 16, 2024

Closing due to inactivity.

@fpgmaas fpgmaas closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deptry does not work when installed globally
4 participants